fix: Allow hooks to be removed after class binding #2422
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PROFeNoM
added
🐛 bug
realFlowControl
approved these changes
Dec 13, 2023
There was a problem hiding this comment.
The tests are looking good and are ✅
@morrisonlevi knows more about the tracer, so if it is okay, let's wait for his review
morrisonlevi
approved these changes
Dec 13, 2023
There was a problem hiding this comment.
I'm not sure about the intricacies about removing hooks and such. I've reviewed code around this before but it's been a while and nothing is fresh. It would take quite some time for me to get back into it. It looks fine to me, but if you want to be extra careful and have me take the extra time to review the deeper impact, let me know.
PROFeNoM
force-pushed
the
alex/fix/remove-hook
branch
from
2ae6d6d to
10f1b5b
Compare
2 tasks
|
Superseded by #2449. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
During the micro benchmarking process, I came across an unexpected behavior related to #2367. The goal of the latter was most notably to introduce minimal overhead when injecting logs while instrumenting
Psr\Log\NullLogger. However, the results showed otherwise (refer to the Before/After comparison below).The problem arises when the call to
\DDTrace\remove_hookis made after a class bind (zen_compile.c:do_bind_class). As a result, the observers-related code (zend_observer.c: _zend_observer_class_linked_notify) does not get executed. This implies thathook.c:zai_hook_resolve_classandhook.c:zai_hook_is_excludedare not called either. Thus, when the supposedly removed class is called again, there is no check to see if the hook was removed or not and we directly go tozend_observer.c:_zend_observe_fcall_begin&hook.c:zai_hook_continue. This pull request addresses this issue by adding the necessary check.From a general perspective, it was only effective to remove a hook before a class binding occurs. Attempting to remove a hook at any other point was ineffective.
While microbenchmarks are still being developed, the before/after comparison below shows the effect of logs injection (Injection) versus no logs injection (Baseline):
Before:
After:
Reviewer checklist